-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Mh/more info feedback component testing #2810
Conversation
Removed vultr server and associated DNS entries |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests look good here, thanks for picking this up!
One comment about our error handling pattern on feedback input, which I'm happy to be picked up separately.
}); | ||
|
||
expect(getByTestId("userCommentTextarea")).toHaveAttribute("required"); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test highlights something interesting - while we can't test that the browser stops form submit in Jest, we'd normally test that an input error message becomes visible when trying to proceed without filling out required fields - but it doesn't seem like we've accounted for that with any of these components and are relying on default browser behavior?
I'd expect us to the use the same GOV.UK pattern of red left border and message that we use for all other public-facing inputs - is there a reason we aren't? I think it's very likely this will be picked up in a11y testing as inconsistent and insufficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the time this feature was being built I think there were conversations about using the native required
although to be fair that might have been scoped to the editor rather than the public view.
I did have a version with the gov uk style errors but they could never actually be seen by users as the required
attribute stopped form submission which would be run to show these validation messages so that state could never be reached.
As this was the case at the time I thought I'd reduce complexity but not showing any code which I thought a user could never see.
It'll be really interesting to see what the a11y testing comes back with because on one hand not using required
for a required field seems somewhat off? Although as you say it's inconsistent 🤔
What:
MoreInfoFeedback
componentFeedbackForm
to add adata-testid
aria-label
for inputs which don't have an explicitinput label
to meet accessibility requirementsWhy: